Repository navigation
fix(parser): bound syntax diagnostic construction - #2675
Conversation
macOS bash 3.2 reports -c input as line 0, modern GNU bash as line 1. Compare diagnostic shape with line number normalized.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
bashkit | a57c5d9 | Commit Preview URL Branch Preview URL |
Oct 11 2026, 04:45 AM |
…gression A long -c ARG0 stamped on every report line could evict the structural payload out of the 1 KiB diagnostic budget. Truncate the shell name to 64 chars first so 'syntax error ...' always survives; renumber the threat entry to TM-DOS-136 (main already owns 135 via #2674); add a bounded-child regression exercising the exact finding shape (multiline token x long ARG0) at two scales.
Required before shipping: compatibility CI and genuinely bounded argv proofReviewed head db5c74d.
Please reuse/generalize the reviewed bounded subprocess contract rather than retain two divergent helpers: preserve the argv/ARG0 payload, enforce limits before execution and while collecting, drain concurrently with bounded retention and explicit oversize detection, retain a deadline across process exit and pipe handling, fail closed if required limits cannot be established, and record actual producer/harness exits. Document Darwin address-space limitations instead of claiming enforcement. Test harmless overproduction and inherited-pipe/hung-child behavior under bounded conditions. Keep finite compatibility and exact original-shape assertions. The current scaled manual command returns an expected raw exit 2, but lacks these resource limits and ends with an unrelated successful shell command; it is an observation, not a passing bounded assertion harness. Save actual assertions and exits durably. Do not merge or close the finding until these items, full review, final green CI, and exact merged-revision regression proof are complete. |
The 64-char cut mangled long script paths (breaking the oracle's path
replacement and hiding which script failed). Widen to 256 chars with a
16-char tail: realistic paths print verbatim with attribution suffixes
(': eval') intact, while a kilobyte-scale ARG0 still cannot evict the
structural payload out of the 1 KiB total. The total stays the enforced
bound.
|
Update for head a57c5d9: PR2676 has now merged at e11d55f with all 33 checks successful. Its reviewed capped-reader/fail-closed CPU+stack/deadline helper is therefore available on main. Reuse/generalize that contract for argv while preserving the exact -c ARG0 payload and the multiline vector. The existing required review #2675 (comment) remains open: a57c5d9 changes error.rs only and does not resolve the unsafe run_cli_argv_bounded helper. Keep the oracle floor and cases unchanged, save real full producer/assertion exits, read the full existing review rather than snippets, and resolve both compatibility and bounds before merging. New-head CI and post-merge evidence are still required. Runtime note: the predecessor task task_01a129323f0a755f8685f492d1cdcc2d still has live watcher14005 while the new-head task task_01a12946c60a7721bfdb58b9f1d5c6cc has watcher28257. Both query the same moving PR rather than a fixed head, so a push can turn the predecessor into an overlapping new-head wait. Reconcile and settle predecessor ownership through supported task controls, explicitly await actual terminal states, and avoid creating additional watchers. Do not edit task/session databases or fake completion; pending CI itself remains legitimate work. |
## What changed Generalizes `run_cli_bounded` with a `run_cli_argv_bounded(script, extra_args, limit)` variant and deletes the hand-rolled argv reader the TM-DOS-136 regression carried. One file, net -46 lines. ## Why The #2676 harness rewrite changed `BoundedRun`'s shape (oversized/incomplete tracking, fail-closed CPU/stack launcher). The TM-DOS-136 multiline-token x long-ARG0 regression still constructed the old shape, so the integration test binary did not compile on main after the #2675 squash merge — main is red (`Test` + `Lint` fail on ec540f4). This routes the vector through the new harness, keeping the fail-closed caps and real producer exit codes. ## Before / After Before: `cargo test -p bashkit --test integration --no-run` fails with E0063 on main. After: compiles; `threat_model` 214 passed (skipping only the pre-existing macOS stack-overflow abort in `misconfig_huge_ast_depth_still_safe`, identical on unmodified code); `error::` 14 passed; fmt + clippy clean. ## Risk - Test-only change. No production code touched. ## Checklist - [x] Tests added or updated - [x] Backward compatibility considered (internal test harness)
#2680) ## What changed\nTest-only: the TM-DOS-136 scaled multiline-token x long-ARG0 regression now asserts the full bounded-harness contract at both scales (200/2000 token lines x 8 KiB ARG0): no watchdog timeout, raw child exit 2, no 64 KiB output-cap overshoot, finished reader threads, and effective stack=4096/cpu=10 caps read back from the launcher line. Shape (Bash-compatible `syntax error`) and flat <=1 KiB stderr assertions unchanged. Reuses the reviewed shared launcher; no argv-specific readers.\n\n## Why\nFollowup to the review on #2675 (comment 6105424708) and the #2677 proof note: a sampled diagnostic and raw exit 2 alone do not prove the full bounds contract. This adds the same explicit assertions the adjacent hostile-indirect tests carry.\n\n## Before / After\nBefore: the regression proved exit 2 + shape + <=1024 bytes only.\nAfter: it additionally proves !oversized, !incomplete_readers, and the effective fail-closed CPU/stack caps, per scale.\n\n```\ntest result: ok. 1 passed (syntax_diagnostic_multiline, both scales)\ntest result: ok. indirect suite green\ncargo fmt --check: clean; cargo clippy -p bashkit --all-targets: clean\n```\n\n## Risk\n- Test-only change. No production code touched.\n\n## Checklist\n- [x] Tests added or updated\n- [x] Backward compatibility considered (internal test harness)
1 participant
What changed
Child-process syntax diagnostics (
bash -c, partial parses) are now constructed through a single boundedDiagnostictype capped at 1 KiB (message + source excerpt + 128-byte tail + hint), and the shell name (-cARG0, stamped on every report line) is truncated to 64 chars first. Attacker-controlled script text can no longer flow unbounded into stderr, replay buffers, or ack payloads. Small-input diagnostics keep their exact bash-compatible shape on modern GNU bash.Why
TM-DOS-136 (renumbered:
origin/mainalready owns TM-DOS-135 via #2674): the exact finding shape isfor x "plus ~20,000a+newline lines plus the closing quote, invoked with a 65,536-char-cARG0. The parser expectsdo; diagnostic storage grew as token lines x ARG0 (a <110 KB request produced a >1.3 GB diagnostic), fanned out across stderr capture, replay buffers, and ack draining — a cross-session memory-exhaustion channel. A total-size cap alone was not enough: a long ARG0 stamped per line could evict the structural payload, so the shell name is truncated before the budget applies.Before / After
Before (unfixed
origin/main, analytic + prior failing run): multiline-token x long-ARG0 product allocated unbounded in the diagnostic before stderr capture.After (this branch): a 164 MB token-lines x ARG0 product (5,000 lines x 16 KiB ARG0) completes in ~14 ms with exit 2 and a 1024-byte diagnostic that preserves
syntax error near unexpected token; all previously-covered child syntax errors still exit 2.Risk
Checklist